test(benchmarks): improve bechmarks to have complete performance data #326

0xarcano · 2024-09-11T20:59:13Z

Description

Added the missing Sparse Merkle tree, the missing operations:

Added Sparse Merkle Tree to the benchmark suites to complete the available merkle tree implementations in zk-kit.
Added operation to the benchmark suites (proof generation, proof verification, update, delete
Changed the "imt.ts" name to "benchmark-merkle-trees.ts"
Added README.md documentation about the benchmark suite
Added log managment with winston library to handle any error produced during the benchmark withtout break the benchmark execution

Related Issue(s)

Closes #311

Other information

The results folder is present in gitignore then the results are not included in the pull request, but you can use the README.md instructions to generate it.

Checklist

It would be helpful to write a comparison of of all them, answering questions like

"when not to use"/"when to use"
"used by?": protocol xyz, app xyz...
include benchmarks: maybe one merkle tree is faster at generating a proof, but slower to update?

- Added Sparse Merkle Tree to the benchmark suites to complete the available merkle tree implementations in zk-kit. - Added operation to the benchmark suites (proof generation, proof verification, update, delete - Changed the "imt.ts" name to "benchmark-merkle-trees.ts" - Added README.md documentation about the benchmark suite - Added log managment with winston library to handle any error produced during the benchmark withtout break the benchmark execution re privacy-scaling-explorations#311

0xarcano · 2024-09-11T21:09:09Z

Dear @sripwoud, I have made the changes and run the recommended yarn commands before commit, but I have received the error:
"YN0028: The lockfile would have been modified by this install, which is explicitly forbidden."

Please give me some clues about how to solve it, I used node 20.17.0 and yarn 4.1.0, when I run yarn to install the dependencies the yarn.lock has been changed but I didn't include it to the commit because I thought that is not convenient.

…alidation issues in GH remove package winston and to-hex libraries to fix validation issues in GH re privacy-scaling-explorations#311

0xarcano · 2024-09-13T21:19:48Z

Hello, I have already fixed the problems in validation of the PR.

sripwoud

Nice!
Small requests for changes.

it would be nice to add a summary results table to the root README (and update the conclusion if necessary based on the analysis).

Eg https://gist.github.com/sripwoud/06a1602fe8dd41c72f39c317cd1676f5#file-summary-md

benchmarks/benchmark-merkle-trees.ts

benchmarks/README.md

benchmarks/benchmark-merkle-trees.ts

benchmarks/README.md

benchmarks/index.ts

benchmarks/benchmark-merkle-trees.ts

Added benchmarks summary tables for different sizes of samples and conclusion ideas re # 311

Co-authored-by: sripwoud <[email protected]>

…dex.ts Added winston to package.json and refactored index.ts to run the samples with a loop managed by an array re privacy-scaling-explorations#311

Co-authored-by: sripwoud <[email protected]>

Changed to initialize from -1 instead of 0 to fix the logic to start from the first samples element re privacy-scaling-explorations#311

…chmarks' into docs/merkel-tree-bechmarks

0xarcano · 2024-09-21T17:33:53Z

I have already took action in all the requested changes, please @sripwoud let me know how it looks

sripwoud · 2024-09-22T08:59:35Z

@prodasl

YN0028: The lockfile would have been modified by this install, which is explicitly forbidden.

is this solved, or do you still need help with this?

sripwoud

nACK

One cannot install the deps with yarn or run yarn scripts (like yarn benchmarks) due to the lockfile's broken indentation.

yarn.lock

benchmarks/README.md

0xarcano · 2024-09-23T14:53:44Z

@prodasl

YN0028: The lockfile would have been modified by this install, which is explicitly forbidden.

is this solved, or do you still need help with this?

0xarcano · 2024-09-23T15:45:45Z

@prodasl

YN0028: The lockfile would have been modified by this install, which is explicitly forbidden.

is this solved, or do you still need help with this?

Hi, @sripwoud, thanks for the support.
I have some doubts about it yet, it is related to my try to send the changes on yarn.lock, because the first time I did the PR, I haven't sent the changes of yarn.lock, but this time I have a lot of change reported in the yarn lock related to packages versions, the only real change was the winston lib added, then my questions are:

Should I include the changes of yarn.lock or definitely it should not be part of the PR? (when I not included I got the mentioned problem YN0028)
If I include the yarn.lock, should I include the package version changes or just the new line for winston lib?

…n script Change the usage instructions of benchmarks from the npx command to use the exisiting script in the package.json re privacy-scaling-explorations#311

…chmarks' into docs/merkel-tree-bechmarks

…lidation Minimal change in the code to add an space to a instruction from prettier lib suggestion

after the run the yarn command in my local environment plus add the new dev dependency winston the yarn.lock have changed some lib versions and added winston re privacy-scaling-explorations#311

0xarcano · 2024-09-23T16:51:05Z

HI @sripwoud ,

Finally, I tried sharing the yarn.lock as it is in my local environment, sorry for all the trouble but I have not worked with those files before, all the checks in Github looks like are passed and all the change requests covered.

Please let me know if it is ready. (thanks again)

sripwoud · 2024-09-23T17:23:01Z

@prodasl

YN0028: The lockfile would have been modified by this install, which is explicitly forbidden.

is this solved, or do you still need help with this?

Hi, @sripwoud, thanks for the support.

I have some doubts about it yet, it is related to my try to send the changes on yarn.lock, because the first time I did the PR, I haven't sent the changes of yarn.lock, but this time I have a lot of change reported in the yarn lock related to packages versions, the only real change was the winston lib added, then my questions are:

Should I include the changes of yarn.lock or definitely it should not be part of the PR? (when I not included I got the mentioned problem YN0028)

If I include the yarn.lock, should I include the package version changes or just the new line for winston lib?

TLDR: you should commit any changes to yarn.lock every time you change deps in package.json

If there are more changes in yarn.lock than strictly related to your PR it is okay and normal: it is due to yarn bumping deps automatically according to semantic versioning rules.

In practice devs exceptionally review changes in yarn.lock because there are always so huge. Reviewing package.json changes is enough.

But avoid modifying yarn.lock yourself. Stick to the yarn add/remove commands.

If it happens that there a mismatch between the committed yarn.lock and package.json then you get the error:

YN0028: The lockfile would have been modified by this install, which is explicitly forbidden.

sripwoud

tACK (successfully ran yarn benchmarks locally)

Thanks @prodasl ! Nice job.

sripwoud · 2024-09-25T07:53:10Z

benchmarks/README.md

+## Table of Contents
+
+-   [Installation](#installation)
+-   [Usage](#usage)
+-   [Benchmark](#benchmark)
+-   [Results](#results)
+-   [Dependencies](#dependencies)


nit:
nice but unnecessary.
GitHub web ui automatically adds a button to see the table of contents for all .md files 😉
https://github.blog/changelog/2021-04-13-table-of-contents-support-in-markdown-files/

0xarcano requested review from cedoor and sripwoud as code owners September 11, 2024 20:59

fix(benchmarks): remove package winston and to-hex libraries to fix v…

81093d8

…alidation issues in GH remove package winston and to-hex libraries to fix validation issues in GH re privacy-scaling-explorations#311

sripwoud requested changes Sep 16, 2024

View reviewed changes

sripwoud assigned 0xarcano Sep 16, 2024

0xarcano and others added 10 commits September 16, 2024 18:58

docs(benchmarks): added benchmarks summary

1c1e108

Added benchmarks summary tables for different sizes of samples and conclusion ideas re # 311

Update benchmarks/README.md

83deb40

Co-authored-by: sripwoud <[email protected]>

Update benchmarks/benchmark-merkle-trees.ts

5e76b88

Co-authored-by: sripwoud <[email protected]>

Update benchmarks/README.md

50054f3

Co-authored-by: sripwoud <[email protected]>

Update benchmarks/benchmark-merkle-trees.ts

f42c8bd

Co-authored-by: sripwoud <[email protected]>

Update benchmarks/README.md

8b761ea

Co-authored-by: sripwoud <[email protected]>

refactor(benchmarks): added winston to package.json and refactored in…

93efa6b

…dex.ts Added winston to package.json and refactored index.ts to run the samples with a loop managed by an array re privacy-scaling-explorations#311

Update benchmarks/benchmark-merkle-trees.ts

3fa14a7

Co-authored-by: sripwoud <[email protected]>

fix(benchmarks): added counter initialization mistake

1585210

Changed to initialize from -1 instead of 0 to fix the logic to start from the first samples element re privacy-scaling-explorations#311

Merge remote-tracking branch 'refs/remotes/origin/docs/merkel-tree-be…

e4989dc

…chmarks' into docs/merkel-tree-bechmarks

sripwoud requested changes Sep 22, 2024

View reviewed changes

yarn.lock Outdated Show resolved Hide resolved

benchmarks/README.md Outdated Show resolved Hide resolved

0xarcano closed this Sep 23, 2024

cedoor reopened this Sep 23, 2024

Merge branch 'main' into docs/merkel-tree-bechmarks

6af06df

0xarcano added 4 commits September 23, 2024 10:46

docs(benchmarks): changed the usage instructions to point the exisiti…

f981010

…n script Change the usage instructions of benchmarks from the npx command to use the exisiting script in the package.json re privacy-scaling-explorations#311

Merge remote-tracking branch 'refs/remotes/origin/docs/merkel-tree-be…

6bd356e

…chmarks' into docs/merkel-tree-bechmarks

style(benchmarks): benchmark-merkle-trees.ts changes from prettier va…

f761603

…lidation Minimal change in the code to add an space to a instruction from prettier lib suggestion

fix(benchmarks): added the new version of yarn.lock

7710579

after the run the yarn command in my local environment plus add the new dev dependency winston the yarn.lock have changed some lib versions and added winston re privacy-scaling-explorations#311

sripwoud approved these changes Sep 25, 2024

View reviewed changes

sripwoud added the documentation 📖 Improvements or additions to documentation label Sep 25, 2024

sripwoud merged commit 9adf8bf into privacy-scaling-explorations:main Sep 25, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(benchmarks): improve bechmarks to have complete performance data #326

test(benchmarks): improve bechmarks to have complete performance data #326

0xarcano commented Sep 11, 2024

0xarcano commented Sep 11, 2024

0xarcano commented Sep 13, 2024

sripwoud left a comment

0xarcano commented Sep 21, 2024

sripwoud commented Sep 22, 2024

sripwoud left a comment

0xarcano commented Sep 23, 2024

0xarcano commented Sep 23, 2024

0xarcano commented Sep 23, 2024 •

edited

Loading

sripwoud commented Sep 23, 2024

sripwoud left a comment

sripwoud Sep 25, 2024

test(benchmarks): improve bechmarks to have complete performance data #326

test(benchmarks): improve bechmarks to have complete performance data #326

Conversation

0xarcano commented Sep 11, 2024

Description

Related Issue(s)

Other information

Checklist

0xarcano commented Sep 11, 2024

0xarcano commented Sep 13, 2024

sripwoud left a comment

Choose a reason for hiding this comment

0xarcano commented Sep 21, 2024

sripwoud commented Sep 22, 2024

sripwoud left a comment

Choose a reason for hiding this comment

0xarcano commented Sep 23, 2024

0xarcano commented Sep 23, 2024

0xarcano commented Sep 23, 2024 • edited Loading

sripwoud commented Sep 23, 2024

sripwoud left a comment

Choose a reason for hiding this comment

sripwoud Sep 25, 2024

Choose a reason for hiding this comment

0xarcano commented Sep 23, 2024 •

edited

Loading